fix(pi): hide operational shipshape acknowledgements in Calm - #9
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Implement the captain-approved second presentation layer for Firstmate Pi Calm without substituting presentation hiding for upstream suppression. With Calm on, hide only the exact whole assistant acknowledgement
Captain, shipshape.when it belongs to a canonically classified Firstmate operational user input, using the existing operational classifier and supported Pi transcript/lifecycle identity; preserve both ordinary messages unchanged in model context, delivery, ordering, authority, persistence, session data, export, and share. With Calm off both rows render normally; the phrase after a real human message and every near match including punctuation, prefixes, suffixes, Markdown, additional explanation, capitalization, or whitespace remain visible, as do every substantive reply, decision, failure, blocker, credential request, PR result, investigation finding, tool call, reasoning state, and interruption result. Prevent streaming flash while revealing a reply immediately when it diverges, preserve existing operational-user, startup chrome, tool, thinking, working-boat, activity, queue, and interruption behavior, and keep the change independently correct from the separately validating upstream working-notification suppression branch. Add executable regressions for the positive case, Calm off, human collision, all near-match classes, substantive output, queued operational inputs, intervening tools, reasoning, interruption, and session replay/export preservation where supported. Update the single authoritative Calm contract with exact scope and limits, keep tracked prose one sentence per line, and preserve every Firstmate safety boundary. Independent baseline evidence established that the unchanged primary branch has the same native Calm/export did not complete while calm mode was ontimeout after all preceding checks pass; preserve that evidence, do not modify export behavior, and do not loosen or skip its test.What Changed
Captain, shipshape.when it answers a canonically classified operational user input, scoping the origin decision to whole agent runs and re-deriving it per row during transcript rebuilds so acknowledgements after genuine captain messages, near matches, and all substantive replies stay visible with no streaming flash.docs/calm.md,docs/calm-mode-feasibility.md, and the README were aligned with the three-adapter set and the acknowledgement-hiding contract, andtests/fm-calm-pi-extension.test.shgained regressions for the positive case, Calm off, human collisions, near-match classes, queued operational inputs, intervening tools, interruption, replay, and per-adapter degradation.Risk Assessment
✅ Low: The final commit is a two-file documentation and test-enumeration alignment, and the branch as a whole is a well-bounded, presentation-only Pi Calm change that leaves message data, delivery, persistence, export, and share provably untouched, fails open on every ambiguous path, and carries executable regressions for each acceptance criterion.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
⏭️ **Rebase** - skipped
.agents/skills/afk/SKILL.md- branch carries 164 commit(s) that exist on your local main branch but were never pushed to origin/main; rebasing would bundle this unrelated work (293 file(s)) into the PR:Push main to origin, or rebase your branch onto origin/main, before gating.
docs/calm.md:19- docs/calm.md:19 states unconditionally thatCaptain, shipshape.after a genuine captain message remains visible, but the acknowledgement is bound to the most recently added user chat row (fm-calm-operational-user-layout.ts:137 -> fm-calm-assistant-layout.ts:106-110), not to the specific input the response answers. Pi's steer/followUp path emits a user message_start for an operational wake injected into an already-running human turn, so a later assistant component created in that same run (for example after a tool boundary) captures origin=operational and an exact acknowledgement is hidden even though a genuine captain message is in the run. The primary ordering is implemented and tested correctly; only this same-run interleaving diverges from the stated absolute. Either qualify the contract sentence or re-derive origin per assistant message..pi/extensions/lib/fm-calm-assistant-layout.ts:26- .pi/extensions/lib/fm-calm-assistant-layout.ts:26-30 and .pi/extensions/lib/fm-calm-operational-user-layout.ts:45-49 renamed both Symbol.for patch keys from:pi-0.81.1to:operational-ack-v1while retaining the comment "Keep the introduction-version symbol stable so a compatible upgrade cannot double-patch a live process." The rename is correct (the patch shape gained assistantOperationalOrigins and currentInputIsOperational, so reusing the old key would leave the old closure active), but the retained comment now asserts an invariant the change deliberately breaks. A process loading a stale adapter copy alongside the updated one registers under two keys and double-wraps updateContent/addMessageToChat; the double wrap is benign here, but the comment misleads a future maintainer. Reword the comment to say the symbol changes only when the patch shape changes.docs/calm-mode-feasibility.md:215- docs/calm-mode-feasibility.md:215 and both adapter header comments dropped the "Verified against Pi 0.81.1 and 0.82.0" scope, while the same document still asserts "verified on Pi 0.81.1 through 0.82.0" at lines 69, 127, and the policy-table header at line 191. That document is declared the owner of the version-scoped renderer taxonomy and empirical evidence, so the unscoped and scoped claims cannot both be authoritative. Either restore the version scope at line 215 or remove it from lines 69, 127, and 191 as well.🔧 Fix: Scope Calm acknowledgement hiding to whole agent runs
2 issues (1 warning, 1 info) still open:
.pi/extensions/lib/fm-calm-assistant-layout.ts:51- .pi/extensions/lib/fm-calm-assistant-layout.ts:51-53 AND-accumulates every user row whenever a run is active, but Pi rebuilds the whole chat inside the run on auto-compaction. agent-session.js:744-756 keeps _runAgentPrompt in its try block and only emits agent_settled from the finally, so _handlePostAgentRun -> _checkCompaction -> _runAutoCompaction emits compaction_end and interactive-mode.js:2503-2505 runs chatContainer.clear() + rebuildChatFromMessages() while runIsActive and runOriginRecorded are both still true. Every replayed historical user row then takes the AND branch, the first genuine captain row in history drives runOriginIsOperational to false, and replayed AssistantMessageComponents created after it capture origin=false - so previously hiddenCaptain, shipshape.rows reappear in the rebuilt transcript, and the agent.continue() continuation run also renders its acknowledgement. Commit 812b55c handled this correctly because each replayed row set the flag directly; the header comment at line 10 ("Outside a run each replayed row stands alone") assumes replay never happens inside a run. Failure direction is fail-open (extra rows visible, never wrongly hidden). Suggested direction: mark a replay window by probing renderSessionItems/renderSessionEntries and score rows per-row inside it while leaving the run accumulator untouched..pi/extensions/lib/fm-calm-assistant-layout.ts:56- .pi/extensions/lib/fm-calm-assistant-layout.ts:56 sets runOriginRecorded = patch.runIsActive, which only produces AND-accumulation because Pi emits agent_start before the run's initiating user message_start (agent-loop.js:49-56: agent_start -> turn_start -> per-prompt message_start). If a future Pi inverted that order, runOriginRecorded would stay false for the run's first input and a later steered operational wake would overwrite runOriginIsOperational to true instead of AND-ing it, silently restoring the bug this commit fixes with no diagnostic. The ordering is pinned by the new regression at tests/fm-calm-pi-extension.test.sh, but the adapter header comment at lines 7-10 should state the dependency the way every other Calm adapter names the exact Pi behavior it relies on.🔧 Fix: Preserve Calm acknowledgement origin across transcript rebuilds
2 infos still open:
.pi/extensions/lib/fm-calm-assistant-layout.ts:46- .pi/extensions/lib/fm-calm-assistant-layout.ts:31-40 adds replayDepth and replayOriginIsOperational to CalmAssistantLayoutPatch, and .pi/extensions/lib/fm-calm-operational-user-layout.ts:165-176 adds a renderSessionItems wrapper to the installed set, but both Symbol.for keys stay:operational-ack-v1(assistant layout line 46, operational-user layout line 56). That contradicts the rule stated directly above each key: "The symbol changes only when the patch shape changes, so a compatible upgrade cannot double-patch a live process and an incompatible one cannot keep a stale closure installed under the same key." In a mixed-version in-process load theinstalledshort-circuits (assistant layout lines 109-114, operational-user layout lines 84-89) keep the previous closure and never install the renderSessionItems wrapper; the surviving patch object has no replayDepth field, so beginCalmTranscriptReplay'sreplayDepth === 0guard is false and it computesundefined + 1= NaN, noteCalmTranscriptUserMessage'sreplayDepth > 0stays false, and endCalmTranscriptReplay's=== 0guard never fires so the counter stays NaN permanently - the replay fix delivered by this commit silently does not apply. Reachability is low (Pi loads extensions once per process), and every consequence is fail-open, but the fix is mechanical: bump both symbols to -v2..pi/extensions/lib/fm-calm-operational-user-layout.ts:105- .pi/extensions/lib/fm-calm-operational-user-layout.ts:104-107 throws before any patching when InteractiveMode.renderSessionItems is absent, so a future Pi that keeps addMessageToChat but drops or renames that internal helper causes fm-calm.ts to skip the entire operational-user-row adapter - losing zero-height operational user rows (a shipped, user-visible Calm feature) and the acknowledgement origin association, not just the replay refinement. Without the replay window the only degradation would have been per-row replay scoring, which is fail-open. Probing the replay seam separately and skipping only that wrapper would preserve row hiding, which is closer to the stated per-adapter degradation philosophy. docs/calm.md:38 documents the coupling as intended and tests/fm-calm-pi-extension.test.sh pins it, so this is a deliberate design call rather than an oversight.🔧 Fix: Probe Calm transcript replay seam as its own adapter
2 infos still open:
docs/calm-mode-feasibility.md:215- docs/calm-mode-feasibility.md:215 still says Calm uses "separate idempotent, API-probed adapters for assistant layout and the complete operational-user transcript row" - two adapters - while docs/calm.md:38 now names three (collapsed-thinking, operational-user-row, transcript-replay), each probing its own seam. The synthetic-assistant table row at line 211 also states unconditionally that "rows replayed through a transcript rebuild stay scored per row even when the rebuild happens inside a run", but docs/calm.md:40 correctly qualifies that: losing only the transcript-replay adapter makes a rebuild inside a run fall back to run scoping, which only makes more replies visible. calm-mode-feasibility.md is declared the owner of the version-scoped renderer taxonomy and empirical evidence, so both documents cannot be authoritative on the adapter set and the replay guarantee. Update line 215's enumeration and qualify line 211's claim to match the compatibility contract.tests/fm-calm-pi-extension.test.sh:329- tests/fm-calm-pi-extension.test.sh:329-332 enumerates only ["collapsed-thinking", installCalmAssistantLayout] and ["operational-user-row", installCalmOperationalUserLayout] in the missing-class-export degradation fixture, so installCalmTranscriptReplayWindow's InteractiveMode guard (.pi/extensions/lib/fm-calm-operational-user-layout.ts:180) is never exercised. That fixture exports neither InteractiveMode nor AssistantMessageComponent and is the test that proves missing class exports degrade per adapter by name; the surrounding pass message now claims coverage for all the adapter degradation paths. Add ["transcript-replay-window", operational.installCalmTranscriptReplayWindow, "InteractiveMode"] to the existing array - a one-line change that closes the gap.🔧 Fix: Align Calm feasibility doc and coverage with three adapters
1 info still open:
.pi/extensions/fm-calm.ts:9- .pi/extensions/fm-calm.ts:9-11 still says "The collapsed-thinking and operational-user presentation adapters probe the exact API they patch and degrade independently with a diagnostic (see installCalmPresentationAdapter below) if a future Pi removes it", but the same file registers three adapters at lines 107-109: collapsed-thinking, operational-user-row, and transcript-replay-window. docs/calm.md:38 and docs/calm-mode-feasibility.md:215 and 240 were all corrected to three in this branch, leaving this header as the last place asserting the old count - and it is the header of the file that actually owns adapter registration. Extend the sentence to name the transcript-replay adapter.command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.